Allow owner/repo as repoid if unique#421
Conversation
Signed-off-by: Christopher Homberger <christopher.homberger@web.de>
|
Example |
|
|
Rather than escape and unescape owner/repo, the client can split by This would limit to the client the need to parse things, and the API can have a clear route that deals with entities by name, while still allowing for a What do you think? |
|
for pools you can try to parse the argument as a UUID. If it fails, we fall back to friendly name parsing. Decide which API call to make based on the argument, in the client. |
|
@gabriel-samfira How do you regenerate swagger from code? |
|
To regenerate the swagger code, you can run: go generate ./...If you add a new type, you should also update: https://github.com/cloudbase/garm/blob/main/apiserver/swagger.yaml |
| vars := mux.Vars(r) | ||
| repoID, ok := vars["repoID"] | ||
|
|
||
| repo, ok := a.GetRepository(w, r) |
There was a problem hiding this comment.
It's okay to have some duplicate code, if we can skip doing an extra query. There are people using GARM at scale (hundreds of runners per second in some cases), adding extra queries increases DB queries and load.
It's also desirable to have separate functions for getting by ID versus getting by owner/name. Branching removes duplication at the expense of clarity and flexibility.
So having a ListInstancesForRepositoryByNameHandler(), which has a specific route that is tied to it, is okay. It also allows us to refactor it later without touching the bits that use the ID instead of the owner/name.
It also means having a distinct type in the client code generated by swagger. We want to keep the ability to query by ID alone from the client.
So in summary:
- Existing routes and handlers can remain unchanged
- New handlers and routes can be added to handle the cases where names are used instead of IDs
- For the handlers with names, we also need to be able to specify a query arg indicating
forgeTypeand/orendpointNamein case of multiple results.
There was a problem hiding this comment.
It's okay to have some duplicate code
The duplicated code became to much for me to handle. The swagger duplication was an adventure.
e.g. The shared code had a bug, now I could just fix it everywhere by updating a single line
How it was, I had to change 12+ variants, some of them were sightly different.
I have no idea how you can maintain the duplicated code
In Gitea I saw ugly bugs due to duplicated code, e.g. editwebhook was broken for newly added events.
In c++ I used templates once to have 10% of the codebase than required for duplicated code.
Maybe Duplicate the a.GetRepository(w, r) and use a.GetRepositoryID(w, r) here if only repoID is needed skip the db query.
You can always inline selected functions again if you have a need for special handling
There was a problem hiding this comment.
You make good points. My apologies for the long reply. I like in depth discussions on some things, so bare with me (I wouldn't blame you if you just /dev/null the whole thing 😅).
The swagger duplication was an adventure.
It essentially requires a new handler for each route that wants a friendly name variant. The swagger comments are mostly identical, but for the route and the name of the route (used to generate client code). Parameters also have to be declared. But otherwise, it should be straight forward. It's actually more work to de-duplicate than it is to add new handlers.
e.g. The shared code had a bug, now I could just fix it everywhere by updating a single line
The reverse is also true. You can add a bug everywhere by changing a single line. And it's much easier to do, when it becomes difficult to determine the consequences of a change throughout the different branches of code (you need to be intimately familiar with the code to understand how one change, cascades through deeply branched code - I've had this happen on more complex code bases).
I have no idea how you can maintain the duplicated code
It's not that hard. Handlers are atomic. I need to modify one, when I need to change the way a certain endpoint functions. If I don't, there is no point in touching that code. There are places where de-duplicating is great. There are places where duplication is okay. In presentation code like handlers, some duplication is okay. If you need to do a switch in your controller based on which parameters are present, and you delegate to specific functions the processing of that request, the only thing you're de-duplicating is the swagger stuff. And even that becomes ambiguous when transposed into a generated client library.
It's sometimes difficult to decide when it's okay to duplicate and when it's not, but de-duplicating code is much easier to do than splitting up concerns later on, when the code base becomes spaghetti and you can't make heads or tails of what could break if you make a change somewhere.
There is also the matter of allowing for things to diverge in terms of functionality. Let's take the example of getting a repository. In our scenario, if we use the UUID, there is no ambiguity. The ID is unique and it identifies the database entity without issues. If we want to get by name however, we need to take into account the following:
- A repository may have the same owner/name on several endpoints
- Endpoints can be of different types
- There is the potential that in the future, GARM may have the ability to create users which will have their own endpoints defined
- Admin users should be able to view other user defined repos
So when filtering by name, we will probably need to allow API calls to send additional query filters like endpoint, endpoint type, user (if you're an admin, you should specify the user that created the entity), etc. But if you use the same handler for the case where an ID is used, your client still allows setting additional filters for that case. Should they cause an error? Should they be ignored? How would the docs for the client look like?
An API endpoint that filters only by ID will most likely never change. It will try to match that ID against the database and will either return the entity or a 404 or 401 error.
An API endpoint that filters by name, needs to take into account multiple factors, which may change over time. This is due to the inherent ambiguity of filtering by something that is not unique. We need to supply a set of parameters that together, guarantee uniqueness. That set of parameters may change over time (unlike the UUID) based on features we may need to add (perhaps multi user support?).
So from an architectural stand point, we have to weigh the benefits of not having an eye bleed, now, due to looking at duplicate code and constantly fighting the urge to hack it with an axe, against future growth where we can keep what is currently functional, and only touch what we care about, without having to think about breaking anything else.
So you have a GetRepoByIDHandler() that services /api/v1/repositories/{repoID} (with no other options) and a GetRepoByFriendlyNameHandler() that services /api/v1/repositories/{owner}/{name} with potential query args that may change over time. The current one may never change. The other one may be changed independently.
I'd much rather have proper test cases for new code (-sobs in little time to add tests-) if the fear is that by duplicating code, we end up with different behavior between old and new code.
All of that being said, I know time is precious (and I am humbled any time someone chooses to use theirs to help). I do not want to burden you with implementing a feature that may have seemed simple to begin with but due to my preferences, or quirks or whatnot, you end up spending a lot of time adding. If you prefer, I can pick this up sometimes in the next 2-3 weeks (if it's not a blocker for you). The user story you presented here is clear (and much easier to use from a UX perspective).
There was a problem hiding this comment.
skipping the other points for now, since my context is limited
It essentially requires a new handler for each route that wants a friendly name variant.
Doesn't this PR has new client side handlers and request parameters? I forget the new swagger comment in e.g the method you commented on.
The swagger func name is different for repoid + owner/repo and query.
I know about this from Gitea, e.g. just two swagger blocks.
Gitea uses a router middleware for access scoping and {owner}/{repo} is resolved in the middleware.
So api endpoints do not have to care about how to get the repository object or access control until you accept a unique database id in a subresource that does not use repoid in the query.
e.g. poolid does not belong to the repository in the url.
If the name is not unique you currently have to provide endpointName in query, changing this after reduplication is harder
There was a problem hiding this comment.
Doesn't this PR has new client side handlers and request parameters?
If the extra swagger comment generates the proper client, it's fine.
The initial comment on this line, was due to the fact that there was a call to a.GetRepository() which queries the DB for the repo, and then gets the ID of the repo to pass on to ListRepoInstances(), where before we got the ID from the route. That has changed now from what I see 😄, so I will have a closer look when the branch is marked as ready for review.
Edit: I don't want to create chaos while things are still in flux.
* next step would be to remove that again and duplicate everything
* I cannot figure out to run the e2e tests
There are multiple options for you to choose from, since I didn't force push.
Option 1-3 have the exact same api surface and generated client code, code duplication is the only thing that varies. @gabriel-samfira How can I run intergration tests locally on ubuntu 25.04 (older ubuntu arm64 didn't had desktop isos for my VM)? This is like on the keda project, it is expected to have several secrets set without guide how to prepare the resources the tests depend on. Black box tests that are most of the time fine if the CI blindly runs them for me, but are awkward when testing locally. Having an automated Gitea instance that could automatically create the required landscape makes it easier for me. Or some scripts that creates the test orgs and repos automatically. |
my brain is mush (had too little sleep), but I will have a look soon.
That's a bit more complicated, because the integration tests need access to a test org and a test repo. Moreover, due to the nature of runners, I needed a way to expose the GARM API to github so we can receive webhooks. All this makes integration tests more complicated, at least when coupled to github. For Gitea, it should be way easier to implement tbh. For creating the reverse proxy/tunnel I created a quick and dirty service (probably full of bugs given it was a 1 day project) here: https://github.com/gabriel-samfira/localshow To access this, the current integration job needs a secret that contains a SSH private key. Details bellow. But you can also use ngrok (I think there's an action to set it up) or serveo, or FRP, or rathole (not sure if it's maintained anymore). The idea is to temporarily expose the API from behind a NAT somewhere (typical runner setup). The needed secrets are:
The last 2 depend on whether you use the quick and dirty reverse proxy I linked to or not. If you go with something that is actually maintained, you can skip those and just update the job. For gitea you don't even need this. Everything can be on the same machine. Gitea in a container, GARM started up in the background by the tests. I attempted to make it easier to run locally, but the fact that we need github to be able to send us webhooks to make the tests relevant, makes this tough. |
TL;DR; the second option would be desirable My general preference is as follows:
In the above case, it's okay to duplicate the handlers and have one Swagger definition per handler, but de-duplicate any common code that can be used by both the get by ID and by friendly name. |
This reverts commit 827233c.
In this case is not much common, since the old code only has a getRepositorybyid call. The other logic does not seem to be useful for sharing between them if they do not share an handler.
Hmm, I did need GH_REPO, GH_OWNER, GH_TOKEN and much much much more. Even then after 5 obscure errors and hang ups I do not see any way forward on that front. EDIT, I seem to often forget that the tests are called integration here, but they look for me like being an e2e test against an external service |
| WillReturnRows(sqlmock.NewRows([]string{"name", "owner"}).AddRow(s.Fixtures.Repos[0].Name, s.Fixtures.Repos[0].Owner)) | ||
| s.Fixtures.SQLMock. | ||
| ExpectQuery(regexp.QuoteMeta("SELECT * FROM `repositories` WHERE (name = ? COLLATE NOCASE and owner = ? COLLATE NOCASE and endpoint_name = ? COLLATE NOCASE) AND `repositories`.`deleted_at` IS NULL ORDER BY `repositories`.`id`,`repositories`.`id` LIMIT ?")). | ||
| ExpectQuery(regexp.QuoteMeta("SELECT * FROM `repositories` WHERE (name = ? COLLATE NOCASE and owner = ? COLLATE NOCASE) AND endpoint_name = ? COLLATE NOCASE AND `repositories`.`deleted_at` IS NULL ORDER BY `repositories`.`id` LIMIT ?")). |
There was a problem hiding this comment.
Did I break some logic of garm here?
ORDER BY `repositories`.`id`,`repositories`.`id`
Looked weird
| func (suite *GarmSuite) TestRepositoriesByName() { | ||
| t := suite.T() | ||
|
|
||
| t.Logf("Update repo with repo %s/%s", suite.repo.Owner, suite.repo.Name) |
There was a problem hiding this comment.
This is the integration tests I wonder if it passes, should be equal to the TestRepositories, but using the generated clients of the RepositoryByName.
| q = q.Count(&cnt) | ||
|
|
||
| if cnt > 1 { | ||
| return Repository{}, errors.Wrap(runnerErrors.ErrBadRequest, "multiple repositories with the same name and owner found") | ||
| } |
There was a problem hiding this comment.
Does garm allow to create the same endpoint_name for GitHub and Gitea at the same time?
At least your comments about credentials make me believe this is true and a possible problem here
There was a problem hiding this comment.
If you're referring to endpoints, the name of an edpoint (github, ghes, gitea) is the primary key. So, endpoint names are unique (regardless of type). If you define a GHES endpoint as my first forge, you can't define a gitea endpoint with the same name.
If you're referring to whether you can have the same owner/repo on multiple endpoints, then yes. A repo is unique within the context of an endpoint.
There is no reason why github.com/cloudbase/garm and ghes.example.com/cloudbase/garm could not both exist and be managed by GARM.
Sadly, there is no easy way to run them due to the reasons I detailed earlier. The reliance on GitHub and the need to expose the GARM API to github, makes things difficult. I can run them on a fork.
No worries. Once the review is done, you can squash/rebase.
They are end-to-end tests essentially. They started out as integration tests when the project was first created because we needed to test that GARM works well with the external provider we had at the time (LXD). It has since grown, so the tests no longer represent "integration" given that we only test against one of the many providers. But I was too lazy to change the name. There are references to e2e in the suite. I don't care enough about the terminology to change it, given the current bandwidth. |
|
I will try to allocate some time for a proper review in the morning. |
gabriel-samfira
left a comment
gabriel-samfira
left a comment
There was a problem hiding this comment.
Looks good for the most part. Just a few comments and a few things missing.
Here is an integration test run: https://github.com/gabriel-samfira/garm/actions/runs/15487918541/job/43606694806
Sorry it's taking so long to review.
| } | ||
| } | ||
|
|
||
| // swagger:route GET /repositories/{owner}/{repo}/scalesets Repositories scalesets ListRepoByNameScaleSets |
There was a problem hiding this comment.
What do you think about naming these types something like ListRepoScaleSetsByFriendlyName ? Similar to the other ByName functions. ListRepoByNameScaleSets feels a bit complicated to remember.
There was a problem hiding this comment.
Would be regex
([^ \(\.]+Repo)ByName([^ \(]*)(Handler)to$1$2ByFriendlyName$3(Handler)([^ \(\.]+Repo)ByName([^ \(]*)to$1$2ByFriendlyName$3(Swagger)- update tests to use new names
If we name this like ListRepoScaleSetsByFriendlyName, then I would expect that I am able to use a friendly name of a scaleset as well instead of a scaleset id.
If this function would accept both friendly and ids of scalesets in future this name makes sense to me. What do you think?
Naming things is not my strength
There was a problem hiding this comment.
Naming things is not my strength
We share that quality.
If we name this like ListRepoScaleSetsByFriendlyName, then I would expect that I am able to use a friendly name of a scaleset as well instead of a scaleset id.
that's true, and is something we will probably do in the future (I would like to name pools as well at some point), but given that pools (right now at least) can only be referenced by ID, I think it would be sufficiently intuitive that consistency is maintained between pools and scale sets, and should not cause confusion.
When we do add the ability to reference scale sets by name, we can keep the name, handler and URL the same and add aditional query arg options to be able to specify runnerGroup (scale set names are unique within a runner group) and endpoint.
| First(&repo) | ||
| Preload("Endpoint") | ||
|
|
||
| q = q.First(&repo) |
There was a problem hiding this comment.
I think we can move this call to First() after the check for Count(). If we don't err out in that condition, we can then execute the extra SELECT. Otherwise, we spare one query.
| repoObj, err := r.store.GetRepository(ctx, owner, repo, endpointName) | ||
| if err != nil { | ||
| if errors.Is(err, runnerErrors.ErrNotFound) { | ||
| return params.Repository{}, runnerErrors.NewBadRequestError("repository %s/%s (%s) not found", owner, repo, endpointName) |
There was a problem hiding this comment.
There is a runnerErrors.NewNotFound() function. Or we can just return the original error that is already an ErrNotFound. Not finding a repo should not be a bad request.
| } | ||
|
|
||
| func updateRepoByName(apiCli *client.GarmAPI, apiAuthToken runtime.ClientAuthInfoWriter, owner, repo string, repoParams params.UpdateEntityParams) (*params.Repository, error) { | ||
| updateRepoResponse, err := apiCli.Repositories.UpdateRepoByName( |
There was a problem hiding this comment.
You need to add a route in apiserver/routers/routers.go before this will work.
| ///////////////////// | ||
| // Get pool | ||
| apiRouter.Handle("/repositories/{repoID}/pools/{poolID}/", http.HandlerFunc(han.GetRepoPoolHandler)).Methods("GET", "OPTIONS") | ||
| apiRouter.Handle("/repositories/{owner}/{repo}/pools/{poolID}/", http.HandlerFunc(han.GetRepoByNamePoolHandler)).Methods("GET", "OPTIONS") |
There was a problem hiding this comment.
You probably need to add routes for CRUD for repositories as well, not just for repo pools. Same for repo instances.
There was a problem hiding this comment.
Seems like your are right
garm/apiserver/routers/routers.go
Lines 279 to 298 in d0cf7d0
messed this up.
There was a problem hiding this comment.
That's ok. It's what reviews and tests are for.
Co-authored-by: Gabriel <samfiragabriel@gmail.com>
* maybe just remove the whole special not found handlling
|
Let me know when you're done making changes so I can take another look. |
|
@gabriel-samfira I found new design issues with {repoid} => {owner}/{repo} that are not present in my intial In b45d604 I had to change the router definition order, the first sign of a real conflict. Moving this one level up, was somehow ok. Is there another big flaw in the routing here? e.g. /repostitory/{repoid}/webhook Growing apis like this is error prone, do we really want to go this way? Otherwise make this on the cli client, but keep the GetRepositorybyFriendlyName endpoint to grep the id. (no this has a conflict as well, if we do not use something like /repostitoryByFriendlyName/{owner}/{repo} or other prefix) |
There is a potential of conflict if there is ever a
We have a few options here, but I need to have the time and mental clarity to think about this and forward a discussion. Growing the API will eventually lead to conflicts, but we have a couple of options we need to weigh the pros and cons of:
There might be other options, but I can't allocate time now to proper thought/research. So, to conclude. You're absolutely right that this can't grow much further without asking for trouble. The conflict you identified is pretty glaring. If you think that at this point it's easier to have a CLI-only feature that emulates what you tried to add with this PR, I am all for it (with a tracking issue opened to potentially implement this server side in the future, via an option that is clean and correct - which can be implemented by whomever has the cycles). What are your thoughts on this? I am open to anything. |
|
Do you know if we can restrict repoid routes by a guid regex? So the router skips guid routes for named repos. e.g.
|
|
I think it's possible. Worth a try. It might be cleaner to version the API, though. Something like: apiV11SubRouter := router.PathPrefix("/api/v1.1").Subrouter()
apiV11SubRouter.Use(initMiddleware.Middleware)
apiV11SubRouter.Use(urlsRequiredMiddleware.Middleware)
apiV11SubRouter.Use(authMiddleware.Middleware)
apiV11SubRouter.Use(auth.AdminRequiredMiddleware)
apiV11SubRouter.Handle("/repositories/{owner}/{repo}/pools/{poolID}/", http.HandlerFunc(han.GetRepoPoolByFriendlyNameHandler)).Methods("GET", "OPTIONS")
apiV11SubRouter.Handle("/repositories/{owner}/{repo}/pools/{poolID}", http.HandlerFunc(han.GetRepoPoolByFriendlyNameHandler)).Methods("GET", "OPTIONS")
apiV11SubRouter.Handle("/repositories/{owner}/{repo}/pools/{poolID}/", http.HandlerFunc(han.DeleteRepoPoolByFriendlyNameHandler)).Methods("DELETE", "OPTIONS")
apiV11SubRouter.Handle("/repositories/{owner}/{repo}/pools/{poolID}", http.HandlerFunc(han.DeleteRepoPoolByFriendlyNameHandler)).Methods("DELETE", "OPTIONS")
apiV11SubRouter.Handle("/repositories/{owner}/{repo}/pools/{poolID}/", http.HandlerFunc(han.UpdateRepoPoolByFriendlyNameHandler)).Methods("PUT", "OPTIONS")
apiV11SubRouter.Handle("/repositories/{owner}/{repo}/pools/{poolID}", http.HandlerFunc(han.UpdateRepoPoolByFriendlyNameHandler)).Methods("PUT", "OPTIONS")
apiV11SubRouter.Handle("/repositories/{owner}/{repo}/pools/", http.HandlerFunc(han.ListRepoPoolsByFriendlyNameHandler)).Methods("GET", "OPTIONS")
................ The rest of the friendly name URLs ................The only new code in the example above is defining the new Regex might also work for the UUID. I mean, if someone names their repository using |
Is your idea for versioned api to only allow friendly name in v1.1? I mean otherwise this would only move this routing problem from v1 to v1.1. Or doing something similar to Or just keep clean router for v1?
The uuid filter only applies to repository owner not name. Basically |
I assume we agree calling RepoList to query the id is not good. |
The old router would still exist and we don't need to duplicate old routes that exist there, in If the client needs to list pools by ID, it knows the minimum API version for that is So only the friendly name URLs would need to exist (currently) in that router. Whatever exists now in
Yes. I suggest adding a The list repo/org/enterprise handler can look for the query args and adjust the DB query accordingly. This should allow us to return a small subset (min 0, max depending on whether or not So either of these 2 approaches are ok. The client side one with minimal changes to the list entity endpoints seems easier. The API versioning one needs more thought and careful planning. |
Now that I think about it, this would actually be a nice addition. A search function that allows you to filter by owner, endpoint, repo name or all 3 at once. Hah. |
|
I based #432 on your PR and merged it. @ChristopherHX I would like to thank you for pursuing this and for your patience while we figured out how to best solve this. I will close this PR as the same functionality exists in the one I mentioned above. If you believe the above PR is lacking or not quite what you had in mind, feel free to ping back here. We can improve it or replace it. |
2 participants
This one is so much easier on the client, the problem here is owner/repo contains a slash.
The slash is escaped, gorilla and several reverse proxies will merge %2f as
/and change the url target.Disabling this somehow looks awful, and may not work correctly depending what is between garm and the cli client
This does not feel right, unless moving repoid out of the PATH